Skip to content

fix: walk a style's parent chain instead of recursing it, and read an xml part once - #691

Merged
andiwand merged 2 commits into
mainfrom
fix/style-chain-and-xml-buffering
Aug 14, 2026
Merged

fix: walk a style's parent chain instead of recursing it, and read an xml part once#691
andiwand merged 2 commits into
mainfrom
fix/style-chain-and-xml-buffering

Conversation

@andiwand

Copy link
Copy Markdown
Member

🤖 Generated with Claude Code

Two native crash clusters from Play (14 Aug 2026), both document-driven.

Stack overflow in StyleRegistry::generate_style_

A w:basedOn / style:parent-style-name chain was resolved one stack frame per link, so a document declaring enough styles in one chain overflowed the stack of the thread that opened it — an http worker's, which is small. Both registries now walk the chain onto a stack and build it from the root down. The cycle guard is unchanged in meaning: an entry present but still null.

ooxml_text_style_test.cpp resolves a 100 000-link chain on a thread, so it runs on the small stack rather than the process's. Against the old recursive code it kills the test binary; against this it passes in ~85 ms. Also covers inheritance, a cycle, and an unknown parent.

Double buffering in xml::parse

pugixml's stream loader holds every part twice — the chunk list it reads into, then the contiguous buffer it parses in place. Zip entries know their uncompressed size, so the part is read once into a buffer pugixml is handed and frees. This lowers peak memory for the reported pugixml OOM aborts; it does not eliminate them, since Scudo aborts inside malloc rather than returning null.

Verification

  • Full suite: 880 passed, 0 failed — including the reference-output comparison, which is what shows the parse change is byte-identical.
  • clang-format and clang-tidy clean on the changed files.

The third cluster in the same triage (DynamicUnit::to_string() SIGSEGV) needs no change here: it is an unguarded frame.width()-> in v6.5.0, already fixed on main by 734b6cc and not yet released.

… xml part once

Two crashes reported from Play, both on documents rather than on anything
the caller does.

A `w:basedOn` / `style:parent-style-name` chain was resolved one stack frame
per link, so a document declaring enough styles in one chain overflowed the
stack of the thread that opened it - an http worker's, which is small. Both
registries now walk the chain onto a stack and build it from the root down;
the cycle guard stays what it was, an entry present but still null.

Reading a part through pugixml's stream loader holds it twice - the chunk
list it reads into, then the contiguous buffer it parses in place. The file
knows its size, so it is read once into a buffer pugixml is handed and frees.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01WACM9eXR7F5aZ22tyq2jwU

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ef62376ff

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread src/odr/internal/util/xml_util.cpp Outdated
`parse` splits so the buffering lives on an `abstract::File` overload and the
filesystem one opens and delegates.

Opening a zip entry throws when it is encrypted or compressed by a method we
do not have, and the entry's size is its own claim until then - so the stream
is acquired first, and the buffer is held under `unique_ptr` until
`load_buffer_inplace_own` takes it over.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Claude-Session: https://claude.ai/code/session_01WACM9eXR7F5aZ22tyq2jwU
@andiwand
andiwand merged commit 13a6e40 into main Aug 14, 2026
25 checks passed
@andiwand
andiwand deleted the fix/style-chain-and-xml-buffering branch August 14, 2026 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant